Repository navigation
baremetal list: add --details - #2886
Merged
Merged
Conversation
Change the --provision-state choice "managable" to "manageable" in osism baremetal list. Ironic's provision state is spelled "manageable". The misspelled choice was passed to Ironic as a filter that no node can match, so "--provision-state managable" always listed nothing, while the correct spelling was rejected by argparse. Listing the manageable nodes was therefore impossible with this option. Add a test that "manageable" is accepted and forwarded to the node query. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="osism/commands/baremetal.py" line_range="254-257" />
<code_context>
+ row += [
+ _format_state_since(b["provision_updated_at"]),
+ b["target_provision_state"] or "",
+ _format_automated_clean(b["automated_clean"]),
+ _format_maintenance_reason(b),
+ _shorten_last_error(b["last_error"]),
+ ]
result.append(row)
</code_context>
<issue_to_address>
**Node details can inject terminal controls**
When an attacker can cause Ironic to return terminal control characters in a node detail string, the details row passes these strings to `tabulate` unchanged (`_shorten_last_error` only collapses whitespace), so ANSI/OSC sequences reach the operator's terminal and can forge output or trigger terminal actions.
Escape or remove terminal control characters from node-provided strings before printing.
</issue_to_address>
### Comment 2
<location path="osism/utils/ironic.py" line_range="38" />
<code_context>
+ This goes through the same request and microversion negotiation as the
+ SDK's own proxy method; only the resource class differs.
+ """
+ return DetailedNode.list(conn.baremetal, details=True, **query)
</code_context>
<issue_to_address>
**Detailed columns stay empty**
When the user runs `osism baremetal list --details`, `DetailedNode.list` bypasses the baremetal proxy’s detail-list routing, so Ironic returns summary nodes or rejects the unsupported parameter, leaving detail columns empty or making the command fail.
Route the request through the baremetal proxy’s detail-list method.
</issue_to_address>
### Comment 3
<location path="tests/unit/commands/test_baremetal.py" line_range="635-637" />
<code_context>
+ _run_list_details([], [node])
+
+ out = capsys.readouterr().out
+ assert "first line second line" in out
+ assert "x" * 200 not in out
+ assert "..." in out
+
+
</code_context>
<issue_to_address>
**Wrong error truncation width passes**
When the formatter truncates long errors at a width other than 80, `test_list_details_last_error_flattened_and_shortened` checks the prefix, absence of the full 200-character suffix, and presence of `...`; truncating to 25 characters instead of 80 still satisfies all three assertions, so the test passes while errors are cut to the wrong length.
Assert that the displayed error contains the expected 77-character prefix followed by `...` (or test the formatter directly for an exact 80-character result).
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: osism/commands/baremetal.py:257, osism/utils/ironic.py:38, tests/unit/commands/test_baremetal.py:637
ideaship
marked this pull request as draft
November 12, 2026 14:19
Add a --details option to osism baremetal list. It appends five columns to the table; without it the output is unchanged: - State Since (UTC): when the node entered its current provision state, from Ironic's provision_updated_at. After a series of redeploys it shows which nodes have been redeployed and which still run their old installation. - Target State: target_provision_state, i.e. where a node in a transitional state such as "wait call-back" is heading. - Automated Clean: the node's automated_clean. Ironic's null means the node follows the conductor's [conductor]automated_clean, so it is shown as "default" rather than as False. - Maintenance Reason: maintenance_reason, or Ironic's fault (e.g. "power failure") when Ironic put the node into maintenance itself. - Last Error: last_error on a single line, cut to 80 characters. Maintenance Reason and Last Error are free text; last_error in particular can carry messages that originate from a BMC or the deploy agent. Control characters in them (C0 and C1, which include ESC, BEL and CSI as well as line breaks) are replaced with spaces before printing, so an escape sequence prints as plain text instead of reaching the operator's terminal, and the table stays one line per node. The plain node list returns only a few fields, so --details lists the nodes with details=True. openstacksdk's Node resource does not map provision_updated_at: Ironic sends the field, but the SDK drops it while parsing the response. The new osism.utils.ironic module therefore lists through a Node subclass, DetailedNode, that declares the field. It goes through Node.list, the public method behind conn.baremetal.nodes(), which sends details=True to /nodes/detail, so the request and microversion negotiation are the same as the SDK's own. The subclass can be removed once openstacksdk maps the field. The subclass also repeats Node's "uuid" alternate-id declaration: openstacksdk looks the alternate id up only in the class's own __dict__, so a Node subclass without it reports None as every node's id. The module is imported only when --details is given, keeping openstacksdk out of the command module's import. Tests cover the columns and their formatting, the exact 80-character Last Error cell, the control-character handling, and, by running openstacksdk's real list code against a mocked baremetal proxy, that the request goes to /nodes/detail with the filter and the negotiated microversion and that uuid and provision_updated_at survive parsing. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
baremetal-list-details
branch
from
November 12, 2026 14:35
31dc417 to
d9e2cee
Compare
ideaship
marked this pull request as ready for review
November 12, 2026 14:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits:
manageablestate choice.--provision-stateofferedmanagable. Ironic's state ismanageable, so the misspelled choice could never match a node, and argparse rejected the correct spelling. Listing the manageable nodes through this option was impossible.--details. Without the flag, the output is unchanged. With it,osism baremetal listappends five columns:provision_updated_at). After a round of redeploys, this shows which nodes have been redeployed and which still run their old installation.target_provision_state, i.e. where a node in a transitional state such aswait call-backis heading.automated_clean. Ironic's null means the node follows the conductor's setting, so it is shown asdefaultrather thanFalse.fault(e.g.power failure) when Ironic put the node into maintenance itself.last_error, flattened to one line and cut to 80 characters.openstacksdk does not map
provision_updated_atIronic returns the field, but openstacksdk's baremetal
Noderesource has no attribute for it, so the SDK discards it while parsing the response. The newosism/utils/ironic.pytherefore lists nodes through aNodesubclass,DetailedNode, that declares the field. It usesResource.list, the public method behindconn.baremetal.nodes(), so the request and microversion negotiation are the same. There is no raw API call.The subclass also has to repeat
Node'suuidalternate-id declaration. openstacksdk looks the alternate id up only in the class's own__dict__, not in its parent classes, so without that line a subclass reportsNoneas every node's id. A test covers this.Both points look like upstream openstacksdk issues. Once a fixed openstacksdk is in our pin, the subclass can be removed.
Testing
pytest, black, flake8 and mypy pass locally.🤖 Generated with Claude Code